Hey @fmeum, I'm working to import rules_kotlin to central registry.

What I noticed yesterday is that it's better that I use rootpath instead of rlocationpath, because I already have it in the older Bazel version.

What would happen if we modify rootpath to return the same string as rlocationpath?

My guess is that this would break some users who are not using it only for runfiles and are glueing the results. I don't feel sorry for that.

It would allow us to remove legacy_external_runfiles more easily, because it seems rootpath causes the some uses of it.

I'm sure there are some problems with such a plan that you already know about. I'm just thinking it would be nice to have one function for execpath and one function for runfiles path. (location is already deprecated afaik).

@comius The main reason I dislike this is that rootpath is not a good name: First, neither with its current behavior nor with the proposed behavior does it actually return a "root-relative path", as the name would suggest. For a file @repo//pkg:file, you either get ../repo/pkg/file (today) or repo/pkg/file (if we make it behave like rlocationpath), whereas one would probably expect pkg/file.

One could argue that the root in rootpath stands for "root of the runfiles", but then that's super confusing as well since there is both the "root is the working directory" (foo.runfiles/_main) and the "root is the runfiles directory" (foo.runfiles)" interpretation. I've contributed to Bazel for more than a year now but only stopped being confused by this recently.

Since this is so subtle, I imagine quite a number of people to actually rely on glueing stuff together that happens to work. If we break this expectation in the name of simplifying a migration, we might actually make it more difficult. I would rather deprecate rootpath with more drastic wording and then eventually phase it out of the docs.

Now that I think about it more, if rootpath behaved differently between Bazel <6 and Bazel 6, how would you be able to consistently use it in rules_kotlin?

Regarding what to do about rules_kotlin in the meantime: Could you link to an example of runfiles usage in rules_kotlin that you would like to migrate? Your options that work with both Bazel <6 and Bazel 6 include:

1. Pasting native.repository_name() and $(rootpath ...) together, which works if rules_kotlin is not the main repository.
2. Using the general approach of [rules_runfiles](https://github.com/fmeum/rules_runfiles] to turn the paths you care about into Java constants. While verbose, this is the most reliable approach and may thus be ideal for a major ruleset.
3. Introduce a java_library helper that uses the Java runfiles library and call it from Kotlin. Then, use a BCR patch to make this helper use the new runfiles Create API and annotation with Bzlmod only. It is safe to assume that nobody is using Bzlmod with Bazel <6 since that is broken in many ways.

I pushed bazelbuild/rules_kotlin#860, I'm quite happy with current state. I think it's an improvement over previous state of rules_kotlin. The tests pass with old bazel, new bazel with and without bzlmod.

The runfiles library in Kotlin isn't correct. I'm re-using the -D passing of paths, that was already in the rules_kotlin, so it works with the old library.

I also needed FilesProvider's filesToBuild in data runfiles - but found a neat workaround with filegroup that works on older Bazel versions.

Reading your response, if that's the case, I think we should mark rootpath deprecated today. And to smooth out migration we should also cherry-pick rlocationpath into 5.x.x releases.

The runfiles library in Kotlin isn't correct. I'm re-using the -D passing of paths, that was already in the rules_kotlin, so it works with the old library.

Sounds good, although you should be able to use the Java runfiles library just fine.

I also needed FilesProvider's filesToBuild in data runfiles - but found a neat workaround with filegroup that works on older Bazel versions.

Yeah, that's the best workaround I know of. Happy to see this bug go away though :-)

Reading your response, if that's the case, I think we should mark rootpath deprecated today. And to smooth out migration we should also cherry-pick rlocationpath into 5.x.x releases.

The docs I merged as part of this PR already suggest using rlocationpath instead. Feel free to word this more strongly though! Having this cherry-picked would be great. Is a new minor release planned?

Sounds good, although you should be able to use the Java runfiles library just fine.

I think re-use becomes less important when you cross language barrier :) In case you didn't notice, there's a kt version: https://github.com/bazelbuild/rules_kotlin/blob/master/src/main/kotlin/io/bazel/kotlin/builder/utils/BazelRunFiles.kt

I wouldn't dare to try and reimplement annotation trick in kotlin.

The docs I merged as part of this PR already suggest using rlocationpath instead. Feel free to word this more strongly though! Having this cherry-picked would be great. Is a new minor release planned?

cc @meteorcloudy or @Wyverald, who can tell if there's another 5.x.x release planned.

@comius I wasn't aware of it. It does have some issues, I may look into fixing it after the Bazel 6 release.

Probably no more minor releases on the 5.x track, though patch releases could be done if the need arises. We could do one for API deprecation, I guess -- the decision is a bit above my pay grade

And to smooth out migration we should also cherry-pick rlocationpath into 5.x.x releases.

@comius Do we have to have this feature even for non-Bzlmod build? If not, I think it's OK to break Bazel 5 in the Bzlmod build, since it's experimental in 5.

And to smooth out migration we should also cherry-pick rlocationpath into 5.x.x releases.

@comius Do we have to have this feature even for non-Bzlmod build? If not, I think it's OK to break Bazel 5 in the Bzlmod build, since it's experimental in 5.

Yes, runfiles are there also without bzlmod and rlocationpath works without it. The problem I see is when rlocationpath is used it will break Bazel 5 on non bzlmod builds, because the function doesn't exist.

There are repositories out there that want to have support for 2 LTS releases.

I see, then I'm fine with having a 5.4.0 minor release to cherry-pick this feature to make the migration easier for users.

@kshyanashree Do you think we have capacity to do a minor release at the same time, or should we wait after 6.0 settles down?

Sure @meteorcloudy! I believe we can go ahead for 5.4.0 minor release in parallel. Thanks!